Read tables with multi-argument transforms as unknown transforms#3630
Read tables with multi-argument transforms as unknown transforms#3630moomindani wants to merge 3 commits into
Conversation
Per the V3 spec, readers must read tables with unknown transforms, ignoring them. PyIceberg raised on partition or sort fields with more than one entry in source-ids, so such tables failed to load. Model source-ids on PartitionField and SortField, treat multi-argument transforms as UnknownTransform (null partition values, always-true projection), and serialize per spec: source-id for single-argument transforms, source-ids otherwise. Also fix two latent tolerance bugs: unknown transform names sharing a prefix with known ones (e.g. bucketv2[4]) failed to parse, and str(UnknownTransform) returned "unknown" instead of the original name, corrupting metadata on rewrite. Closes apache#3628
A multi-argument field without a transform previously fabricated
UnknownTransform('None') and masked the missing required field; raise
a clear error instead. Assert explicitly that a single-element
source-ids is normalized onto source-id, and only use the list form
of __str__ for genuinely multi-argument fields.
| sources = ", ".join(str(s) for s in self.source_ids) | ||
| else: | ||
| sources = str(self.source_id) | ||
| return f"{self.field_id}: {self.name}: {self.transform}({sources})" |
There was a problem hiding this comment.
Could you also update SortField.__str__?
There was a problem hiding this comment.
Updated SortField.__str__ in 818e6f9 to render all source ids the same way as PartitionField.__str__.
| if self.source_ids is not None and len(self.source_ids) > 1: | ||
| serialized.pop("source-id", None) | ||
| serialized.pop("source_id", None) | ||
| else: | ||
| serialized.pop("source-ids", None) | ||
| serialized.pop("source_ids", None) |
There was a problem hiding this comment.
Why does this block contain underscore fields that the specification doesn't include? We should delete them in my opinion. Same for partitioning.py.
There was a problem hiding this comment.
Agreed, removed them in 818e6f9. For context, they were a defensive measure for model_dump(by_alias=False), where pydantic emits field names (source_id) instead of aliases (source-id) — but since IcebergBaseModel always forces by_alias=True for metadata serialization, that case never produces spec JSON and the extra pops were unnecessary.
2 participants
Closes #3628 (part of #1818)
Rationale for this change
The V3 spec requires readers to read tables with unknown transforms, ignoring them. PyIceberg raised (
Multi argument transforms are not yet supported) on partition or sort fields with more than one entry insource-ids, so loading such tables failed entirely.source-idsonPartitionField/SortFieldand treat multi-argument transforms asUnknownTransform, which already gives the spec behavior (no partition pruning:projectreturns None).source-idonly for single-argument transforms,source-idsonly for multi-argument ones, so specs round-trip faithfully instead of being silently rewritten on the next commit.bucketv2[4]) failed parsing instead of becomingUnknownTransform, andstr(UnknownTransform)returned"unknown"instead of the original name, corrupting the transform name when metadata is rewritten.Evaluating multi-argument transforms on write stays out of scope until the spec defines concrete ones.
Note on other implementations: none currently satisfies the spec's tolerance requirement in full. Java has not implemented
source-ids(its parser requiressource-id), though its transform parsing already matches names exactly andUnknownTransform.toString()preserves the original name — the two tolerance fixes here align PyIceberg with that behavior. iceberg-rust and iceberg-cpp reject unrecognized transform names outright. Thesource-idshandling here follows the spec text directly (partition/sort field JSON: single-argument transforms write onlysource-id, multi-argument onlysource-ids).Are these changes tested?
Yes: eight new tests covering multi-argument parse tolerance (partition and sort), round-trip serialization (
source-idspreserved,source-idomitted, transform name intact), single-element normalization,partition_typeresolution, prefix-colliding unknown transform names, andstr(UnknownTransform)name preservation. All fail without the fix. No new failures intests/table,tests/catalog,tests/utils.Are there any user-facing changes?
Yes: tables using multi-argument transforms now load and scan (without pruning on those fields) instead of raising.
This pull request and its description were written by Claude Fable 5.